Skip to content

Fixed singleton design to fix issues with async initialization. - #21

Closed
darthdie wants to merge 1 commit into
Baseflow:masterfrom
darthdie:master
Closed

Fixed singleton design to fix issues with async initialization.#21
darthdie wants to merge 1 commit into
Baseflow:masterfrom
darthdie:master

Conversation

@darthdie

Copy link
Copy Markdown

I was having issues when my app was loading multiple images at the same time on start-up.

Should also resolve the issues you've mentioned before with synchronized not working as you thought with singletons.

@kioba

kioba commented Jun 28, 2018

Copy link
Copy Markdown

did you have a problem with the _cacheData being null when loading multiple images? btw great singleton fix 👍

@mockturtl

mockturtl commented Aug 2, 2018

Copy link
Copy Markdown

did you have a problem with the _cacheData being null when loading multiple images?

I did. I'll open a separate PR with a quick fix.

Edit: see #34

await synchronized(_lock, () async {
if (_instance == null) {
_instance = new CacheManager._();
await _instance._init();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darthdie @mockturtl , an alternative fix might be to do something like:

final backingInstance = new CacheManager._();
await backingInstance.init();
_instance = backingInstance;

Delaying the assignment to _instance until after the initialization keeps that initial null guard working as designed.

This way we don't have to worry about initializing _cacheData in #34 (and then re-initializing it with the new Map() call), and then you don't end up with a race condition on which map (initial or re-initialized) is hit.

Hm. After thinking about it, I'm confused as to how #34 happens with this PR (original version) in place. AFAICT, it shouldn't?

Anyway, just some thoughts. I do like how this current PR gets rid of the nested null guards, and I mostly started to write this out of interest on whether the _cacheData initialization is necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants